Skip to content

#354 | track publishing failures #357

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Apr 25, 2018
Merged

#354 | track publishing failures #357

merged 7 commits into from
Apr 25, 2018

Conversation

@slayful
Copy link
Contributor

@slayful slayful commented Apr 10, 2018 •
edited by michaelklishin
Loading

Proposed Changes

References #354, followed up with #360.

Types of Changes

  • Bugfix (non-breaking change which references issue Metrics for successful and unsuccessful outgoing publishes #354)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (correction or otherwise)
  • Cosmetics (whitespace, appearance)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

@pivotal-issuemaster
Copy link

pivotal-issuemaster commented Apr 10, 2018

@slayful Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

pivotal-issuemaster commented Apr 10, 2018

@slayful Thank you for signing the Contributor License Agreement!

acogoluegnes
acogoluegnes requested changes Apr 23, 2018
View reviewed changes
Copy link
Contributor

@acogoluegnes acogoluegnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs a test in MetricsCollectorTest (it's just a mock test).

src/main/java/com/rabbitmq/client/MetricsCollector.java Outdated

void basicPublish(Channel channel);

void basicPublishFailure(Channel channel);
Copy link
Contributor

@acogoluegnes acogoluegnes Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the thrown exception as a second parameter. Our implementations wouldn't use it, but external ones could.

@acogoluegnes
Copy link
Contributor

acogoluegnes commented Apr 23, 2018

Thanks for this contribution @slayful! I reviewed the PR, it doesn't need much change. Note this is a breaking change as it adds a method to an existing interface (I changed the PR summary to reflect this).

@slayful
Copy link
Contributor Author

slayful commented Apr 23, 2018

Hey, @acogoluegnes I've addressed the feedback, so if you could take a look that would be great.

I can see that concourse-ci's not passing though I don't have access there. I'm currently running the tests on my localhost, and they're looking good so far.

@slayful
Copy link
Contributor Author

slayful commented Apr 23, 2018

Ah, you already did, thank you! :)

@slayful slayful changed the title #354 | [WIP] track publishing failures #354 | track publishing failures Apr 23, 2018
@slayful slayful mentioned this pull request Apr 23, 2018
Closed
@acogoluegnes
Copy link
Contributor

acogoluegnes commented Apr 24, 2018

@slayful The Concourse failure was something else actually, but it's fixed now. Just waiting a bit more before merging, in case @michaelklishin comes up with something to change.

@michaelklishin
Copy link
Contributor

michaelklishin commented Apr 25, 2018

I at first expected this to include what's apparently in #360. That's fine. Perhaps we should add some docs about what exactly counts as a failure at the moment, since unroutable messages and negative acks will be separate metrics.

@acogoluegnes acogoluegnes merged commit 89177a8 into rabbitmq:master Apr 25, 2018
@slayful slayful deleted the rabbitmq-java-354 branch April 25, 2018 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaelklishin michaelklishin michaelklishin approved these changes

@acogoluegnes acogoluegnes acogoluegnes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@slayful @pivotal-issuemaster @acogoluegnes @michaelklishin